Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to use compiler param file for msvc #2843

Closed
wants to merge 3 commits into from

Conversation

cloudhan
Copy link
Contributor

@cloudhan cloudhan commented May 8, 2023

These changes make the nvcc wrapper for msvc to fully support command file for msvc.

Previously, multiple command is longer than CreateProcessW's limit (32767 characters) errors pop up here and there due to the excessive _virtual_includes introduced by mlir.

This makes

  1. the wrapper to accept a params file, that is, `<wrapper.bat> <params_file>
  2. the wrapper to generate filtered params file (command file as of msvc if you want to search it) for the successive msvc invocation, <params_file> -->filter--> <msvc_param_file>), that is in <wrapper.py>, we call Popen(<msvc>, <msvc_param_file>)

@tpopp tpopp requested review from ddunl and jakeharmon8 May 8, 2023 08:20
@tpopp
Copy link
Contributor

tpopp commented May 8, 2023

@ddunl can you ensure someone from your team is aware of this change how ever much you find appropriate? @jakeharmon8 I think you're the most knowledgeable on this after recent work, so can you share your thoughts on this change, even if you don't do the detailed review?

@cloudhan
Copy link
Contributor Author

cloudhan commented May 8, 2023

Note, the <wrapper.bat> is added to workaround

# msvc_cl_path
#   /
python.exe -B <wrapper.py> [[option1]...]
#          \--- write to file.params ---/

# then invoked with 
python.exe @file.params  # This is ill-formed!!!!!!

# file.params
-B
<wrapper.py>
option1
...

with <wrapper.bat>

# <wrapper.bat>
python.exe -B <wrapper.py> %s

# msvc_cl_path
#   /
<wrapper.bat> [[option1]...]
#               \--- write to file.params 

# then invoked with 
<wrapper.bat> @file.params

# file.params
option1
...

@ddunl
Copy link
Member

ddunl commented May 15, 2023

@yashk2810 does this look good to you/any possible negative effect to JAX?

@yashk2810
Copy link
Contributor

@hawkinsp should probably look at this.

@hawkinsp
Copy link
Member

I have no particular knowledge here. This looks good to me but @jakeharmon8 is probably the right person to review.

Perhaps @tlongeri also?

@cloudhan
Copy link
Contributor Author

How about just trigger one run and see if it goes well? There is no external dependency at all and the logic is trivial as described in the comment #2843 (comment)

@ddunl ddunl added the kokoro:force-run Forces CI to rerun label May 23, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Forces CI to rerun label May 23, 2023
@ddunl
Copy link
Member

ddunl commented May 23, 2023

I think Jake already looked at it and thought he wasn't the right person. In this case I'll try to merge! This one will require manual patching in due to issue with our copybara config with respect to tensorflow, and I'm out sick so likely won't get to it today. Thanks for your PR!!

@ddunl
Copy link
Member

ddunl commented May 25, 2023

Today's my first day back, so I still may not get to patching this by the end of this week. If you check on this PR next week and there's no further activity, please tag me again! Sorry for the delay.

copybara-service bot pushed a commit to google/tsl that referenced this pull request May 30, 2023
Imported from GitHub PR openxla/xla#2843

Merging this change closes #2843

PiperOrigin-RevId: 536435983
@ddunl
Copy link
Member

ddunl commented May 30, 2023

Sorry for the delay, working on landing this now. I'll update with any issues should they arise

copybara-service bot pushed a commit to google/tsl that referenced this pull request May 30, 2023
Imported from GitHub PR openxla/xla#2843

Merging this change closes #2843

PiperOrigin-RevId: 536435983
copybara-service bot pushed a commit to google/tsl that referenced this pull request May 31, 2023
Imported from GitHub PR openxla/xla#2843

Merging this change closes #2843

PiperOrigin-RevId: 536555386
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request May 31, 2023
Imported from GitHub PR openxla/xla#2843

Merging this change closes #2843

PiperOrigin-RevId: 536555386
@cloudhan cloudhan deleted the compiler_param_file branch June 3, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants